CQ: Fix entry missing from cache leading to crash on read #11288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The issue comes from a mechanic that allows us to avoid writing to disk when a message has already been consumed. It works fine in normal circumstances, but fan-out makes things trickier. When multiple queues write and read the same message, we could get a crash. Let's say queues A and B both handle message Msg. * Queue A asks store to write Msg * Queue B asks store to write Msg * Queue B asks store to delete Msg (message was immediately consumed) * Store processes Msg write from queue A * Store writes Msg to current file * Store processes Msg write from queue B * Store notices queue B doesn't need Msg anymore; doesn't write * Store clears Msg from the cache * Queue A tries to read Msg * Msg is missing from the cache * Queue A tries to read from disk * Msg is in the current write file and may not be on disk yet * Crash The problem is that the store clears Msg from the cache. We need all messages written to the current file to remain in the cache as we can't guarantee the data is on disk when comes the time to read. That is, until we roll over to the next file. The issue was that a match was wrong, instead of matching a single location from the index, the code was matching against a list. The error was present in the code for almost 13 years since commit 2ef30dc.
|
The following command should trigger the bug before this patch, and not trigger the bug after this patch: It might need converting to non-zsh shell. The bug can be seen when looking at the logs at the same time ( |
|
While the bug does occur in v3.12.x and earlier, it doesn't result in a crash on read, because those versions would defer the read to the message store process when they couldn't read directly. But we have removed that from v3.13.x by ensuring queues can always read from cache or from disk. Except we didn't expect this bug to pop up. |
|
I had to run PerfTest from the binary distribution like so (note the different while true; do ./bin/runjava com.rabbitmq.perf.PerfTest -f persistent -e server.11288.fanout -t fanout -y 49 -c 1 -qp q-%d -qpf 1 -qpt 50 -qa x-queue-version=2 -ad false -f persistent -csd 2 -s 5000 -z 10; done |
michaelklishin
approved these changes
May 21, 2024
mkuratczyk
approved these changes
May 21, 2024
1 task
michaelklishin
added a commit
that referenced
this pull request
May 21, 2024
CQ: Fix entry missing from cache leading to crash on read (backport #11288)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue comes from a mechanic that allows us to avoid writing to disk when a message has already been consumed. It works fine in normal circumstances, but fan-out makes things trickier.
When multiple queues write and read the same message, we could get a crash. Let's say queues A and B both handle message Msg.
The problem is that the store clears Msg from the cache. We need all messages written to the current file to remain in the cache as we can't guarantee the data is on disk when comes the time to read. That is, until we roll over to the next file.
The issue was that a match was wrong, instead of matching a single location from the index, the code was matching against a list. The error was present in the code for almost 13 years since commit 2ef30dc.
Types of Changes